Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(eslint): supports eslint v9 #1729

Closed
wants to merge 2 commits into from
Closed

Conversation

younggglcy
Copy link
Contributor

@younggglcy younggglcy commented Jun 3, 2024

Rollup Plugin Name: @rollup/plugin-eslint

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers: resolves #1708

Description

This PR makes @rollup/plugin-eslint support eslint v9 by simply bumping the eslint version. Ava tests are adjusted to match its needs. Let me know if I miss something.

@younggglcy younggglcy force-pushed the fix/1708 branch 2 times, most recently from a92a5a5 to 92d6003 Compare June 4, 2024 05:00
@younggglcy younggglcy marked this pull request as ready for review June 4, 2024 05:41
@younggglcy younggglcy requested a review from shellscape as a code owner June 4, 2024 05:41
count += results[0].messages.length;
// eslint-disable-next-line prefer-destructuring
const { message } = results[0].messages[0];
t.is(message, "'x' is not defined.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't modify existing tests - that presents a surface area for regressions. instead, we create new tests with the new conditions we'd like to test. I doubt that ESLint would have introduced a breaking change in an 8.x minor version, so this means that the conditions of the test changed. [unless I'm mistaken in some way] please revert this one and create a new test for the new messages and conditions you'd like to test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I'll change these later

{ message: /Found 1 error/ }
);
});
// test('should fail with enabled throwOnError option', async (t) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this commented out?

fs.readFileSync('./test/fixtures/fixable-clone.js').toString(),
fs.readFileSync('./test/fixtures/fixed.js').toString()
fs.readFileSync('fixable-clone.js').toString(),
`\r\n\r\nfunction foo() {\r\n return true;\r\n}\r\n\r\nfoo();`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here again, new tests are preferred to modifying existing tests' expectations

// eslint-disable-next-line prefer-destructuring
const { message } = results[0].messages[0];
t.is(message, "'x' is not defined.");
const { messages } = results[0];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here again, new tests are preferred to modifying existing tests' expectations

Copy link
Collaborator

@shellscape shellscape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please see the comments I left. there are some clarifications that are needed before we can move this forward

@younggglcy
Copy link
Contributor Author

I'm sorry, but I'm unable to move this forward. If anyone is interested, feel free to make a new PR.

@younggglcy younggglcy closed this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[@rollup/plugin-eslint] New flat config eslint.config.js is not recognized
2 participants